Allow IPv6 in Proxy settings and moving validation out from the UI into the model/ interface #430

pablomartin4btc · 2024-11-01T14:34:19Z

The main purposes of this PR are:

Allow configure proxy server with IPv6 (after testing exhaustively in current bitcoin-qt repo and bitcoind - e.g. IPv6 on Qt; IP validation in Qt)
More importantly: moving the validation logic out from the UI (component/ control/ widget) into the back-end (model/ interfaces). It seemed currently in Qt all this logic (validation, network classes) was coupled to the UI since the beginning (more than 12y ago) instead of using interfaces (introduced much later) that would be the correct thing to do.

Feedback and suggestions are very welcome and will help establish a foundation for leaving business logic out of the UI going forward.

In order to test it, if it's the first time you run ./src/qt/bitcoin-qt you need to go thru the on-boarding flow (start->next->next->next->next->connection settings->Proxy settings) otherwise you need to go to the settings (top right gear icon) then Connection->Proxy settings.

Before this change, only IPv4 address were allow in the value input, now also IPv6 can be entered.

There are some look and feel kind of issues that will be handled on follow-ups (issues: #437 and #438).

vasild

Concept ACK

src/interfaces/node.h

src/node/interfaces.cpp

vasild · 2024-11-02T09:16:05Z

src/node/interfaces.cpp

+
+        CService serv(LookupNumeric(ipAddress, DEFAULT_PROXY_PORT));
+
+        Proxy addrProxy = Proxy(serv, true);


What about unix:/path/to/sock?

As discussed offline, in the upstream repo, support to unix sockets was added (bitcoin/bitcoin#27375) already but as this repo is not refreshed and updated yet, I've created #431 and will do a follow-up on this PR once we get the feature added here.

src/qml/components/ProxySettings.qml

src/qml/controls/IPAddressValueInput.qml

vasild · 2024-11-02T10:12:24Z

src/qml/models/nodemodel.cpp

@@ -166,3 +166,13 @@ void NodeModel::ConnectToNumConnectionsChangedSignal()
            setNumOutboundPeers(new_num_peers.outbound_full_relay + new_num_peers.block_relay);
        });
 }
+
+bool NodeModel::validateProxyAddress(QString ipAddress)


Variable names should use snake_case, so ip_address, but maybe addr_port or even just proxy or proxy_str is a better name, especially if unix:/path/to/socket is to be accepted.

especially if unix:/path/to/socket is to be accepted.

As explained above, unix sockets support will be addressed in a follow-up once the feature is ready in this repo.

vasild · 2024-11-02T10:22:44Z

src/node/interfaces.cpp

@@ -398,6 +399,21 @@ class NodeImpl : public Node
    ArgsManager& args() { return *Assert(Assert(m_context)->args); }
    ChainstateManager& chainman() { return *Assert(m_context->chainman); }
    NodeContext* m_context{nullptr};
+    bool validateProxyAddress(const std::string ipAddress) override


For consistency it would be best if the same rules apply to the -proxy= config and the text that is accepted via the GUI. For the former the validation is happening here:

https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/src/init.cpp#L1140-L1168

and here

https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/src/init.cpp#L1513-L1526

I am not sure how relevant the first check is for the proxy because the second check ends up calling SplitHostPort() as well. Maybe ignore the first, move the second to a new function and call this new function from the GUI and from init.cpp?

Ok, let me do some testing on this. Thanks!

I'm planning to refactor the init.cpp file after adding the persistence for the proxy setting, so the logic aligns with how the value is read and parsed from the settings.json file.

I am not sure how relevant the first check is for the proxy because the second check ends up calling SplitHostPort() as wel

I’ll restore the call to SplitHostPort() as advised offline. This is necessary because, without it, LookupNumeric would apply a default port if none is provided, making an address entered without a port (like 127) appear valid. I discovered this while testing the persistence of values in settings.json (a separate PR, which is nearly finished and will be pushed ASAP). For example, if a user enters 127 in the UI, it’s stored in the file without a port. While adding a default port could work, it might not be transparent for the user.

Another point we could discuss, if necessary, in the "setting persistence" PR (coming soon today) is why an IP like 127 remains valid even when a port is specified. This is because, as discussed with @vasil during his review of bitcoin-core/gui#836, the IP validation is left to the OS (thru CService. For example, $ ping 127 results in PING 127 (0.0.0.127) 56(84) bytes of data, and this behavior aligns with how Qt and bitcoind currently handle IPs.

Simplification idea: always require a port, from the GUI and from the command line and from the config file. Edit: the optional :port part is making it more difficult for developers while (probably) providing near 0 benefit to the users.

pablomartin4btc · 2024-11-06T04:53:16Z

Updates:

Addressed @vasild's feedback; next follow-ups:
- init.cpp refactoring will be done when the setting persistence is added in a separate PR.
- unix socket support related changes will be added in a separate PR once the feature is refreshed from the upstream repo.

pablomartin4btc · 2024-11-10T01:08:23Z

Just to clarify:

this PR is ready for further review as the follow-ups mentioned in the previous updates are going to be implemented after this one is merged.

vasild

Approach ACK 3cb348b

src/qml/controls/IPAddressValueInput.qml

pablomartin4btc · 2024-11-11T23:08:29Z

Updates:

Addressed @vasild's latest feedback:
- Corrected max length of the IP input as suggested.
- Updated regex more in accordance with JS.
Updated top description with test instructions suggested offline by @jarolrod.

vasild

ACK ebabe49

I am not very familiar with the QML side of things, but this change looks ok to me.

Good to have in a followup: unify the checks for valid proxy from init.cpp and from the GUI: #430 (comment)

pablomartin4btc · 2024-11-12T16:30:30Z

Good to have in a followup: unify the checks for valid proxy from init.cpp and from the GUI: #430 (comment)

Thanks @vasild for taking the time to review this PR! I'm working already on the next PR which will have both the persistence logic of the proxy and the refactoring of the init.cpp you suggested.

D33r-Gee

tACK ebabe49 works as expected on WSL Ubuntu 22.04

Allowed to configure the proxy with an IPv6 address and moved the validation out from the UI/ control to the node interface.

pablomartin4btc · 2024-11-14T18:43:04Z

Updates:

Restored the call to SplitHostPort() within validateProxyAddress as explained above. This was previously removed in ebabe49 - same comments thread.
- In order to test it: for example 127 was a correct value in the previous commit while now the UI will indicate it's not due to incorrect format unless user adds the port (+ :65535).

vasild

ACK 4dea7c9

MarnixCroes

4dea7c9

the layout breaks when you paste when the default value (127.0.0.1:9050) is still present:

this doesn't happen when first removing the default value and then pasting.

Maybe add scroll functionality to this page?
If you're not on full screen and paste and get the invalid message, the bottom of the page cannot be accessed anymore as you can't scroll down.
On desktop switching to full screen works, but it might be a bigger problem on mobile.
So simple scroll functionality is good to add to the proxy settings page.

pablomartin4btc · 2024-11-25T19:33:56Z

Thanks @MarnixCroes.

As discussed in our call among the team, both the overlapping (#437) and when the validation occurs (#438 - reverting onTextChanged into onEditingFinished would need a "done" button at the top - opposed to the "<back" button - as @GBKS proposed) will be done on follow-ups.

GBKS · 2024-12-03T15:33:27Z

Check out the updated web prototype to see for a proposal of how this logic could work out.

pablomartin4btc · 2024-12-03T18:33:55Z

Check out the updated web prototype to see for a proposal of how this logic could work out.

I really like it, also the fact that the default is greyed out and it disappears when the user starts typing instead of having to delete it (I think it behaves like that currently) . Many thanks!

johnny9 · 2024-12-04T03:08:12Z

src/node/interfaces.cpp

+    {
+        return std::string(DEFAULT_PROXY_HOST) + ":" + ToString(DEFAULT_PROXY_PORT);
+    }
+    bool validateProxyAddress(const std::string& addr_port) override


validateProxyAddress shouldn't be in this module. It should exist in the nodemodel or possibly in netbase.

I agree, maybe it could be moved deeper into de netbase but I didn't what to touch that at this stage, it can be done later once we update the interfaces upstream.

johnny9 · 2024-12-04T03:10:11Z

src/node/interfaces.cpp

@@ -169,6 +170,27 @@ class NodeImpl : public Node
    }
    void mapPort(bool use_upnp, bool use_natpmp) override { StartMapPort(use_upnp, use_natpmp); }
    bool getProxy(Network net, Proxy& proxy_info) override { return GetProxy(net, proxy_info); }
+    std::string defaultProxyAddress() override


I don't believe this should return a string. I think it should be an in/out Proxy&.

This is just for display purposes, it doesn't make sense to build the object which also involves CService and so on, just to come back with eg ipv4.proxy.ToStringAddrPort();, I'd leave it as it is but can be done on a follow-up if more ppl think the change is required.

johnny9 · 2024-12-04T03:10:33Z

src/interfaces/node.h

@@ -126,6 +129,12 @@ class Node
    //! Get proxy.
    virtual bool getProxy(Network net, Proxy& proxy_info) = 0;

+    //! Get default proxy address.
+    virtual std::string defaultProxyAddress() = 0;


Are these actually default settings values or just values presented to the user? If they are just hints, they can be left as strings in the QML.

These are proper default values. I would leave them here for now, but maybe they can be moved into the netbase later.

GBKS · 2024-12-04T08:10:00Z

also the fact that the default is greyed out and it disappears when the user starts typing instead of having to delete it

In the prototype, the light grey text is a placeholder, not the default. We could still treat it that way if we want.

D33r-Gee · 2024-12-04T17:21:24Z

tACK 4dea7c9 on Ubuntu 20.04 works as expected.

Ubuntu 20.04 screenshot

pablomartin4btc force-pushed the qml-ipaddress-allow-ipv6 branch from 4f485db to 0e181eb Compare November 1, 2024 14:38

vasild reviewed Nov 2, 2024

View reviewed changes

pablomartin4btc force-pushed the qml-ipaddress-allow-ipv6 branch 5 times, most recently from ba648fc to 3cb348b Compare November 6, 2024 04:45

vasild reviewed Nov 11, 2024

View reviewed changes

src/qml/controls/IPAddressValueInput.qml Outdated Show resolved Hide resolved

pablomartin4btc force-pushed the qml-ipaddress-allow-ipv6 branch from 3cb348b to ebabe49 Compare November 11, 2024 21:37

vasild approved these changes Nov 12, 2024

View reviewed changes

D33r-Gee reviewed Nov 12, 2024

View reviewed changes

qml, proxy: Allow IPv6 and move out UI validation

4dea7c9

Allowed to configure the proxy with an IPv6 address and moved the validation out from the UI/ control to the node interface.

pablomartin4btc force-pushed the qml-ipaddress-allow-ipv6 branch from ebabe49 to 4dea7c9 Compare November 14, 2024 18:33

pablomartin4btc mentioned this pull request Nov 14, 2024

Persist Proxy settings #433

Open

vasild approved these changes Nov 15, 2024

View reviewed changes

jarolrod added the enhancement New feature or request label Nov 21, 2024

pablomartin4btc mentioned this pull request Nov 22, 2024

Introducing an improved IP Address and Port input control in ProxySettings component #391

Merged

MarnixCroes reviewed Nov 25, 2024

View reviewed changes

johnny9 reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow IPv6 in Proxy settings and moving validation out from the UI into the model/ interface #430

Allow IPv6 in Proxy settings and moving validation out from the UI into the model/ interface #430

pablomartin4btc commented Nov 1, 2024 •

edited

Loading

vasild left a comment

vasild Nov 2, 2024

pablomartin4btc Nov 5, 2024

vasild Nov 2, 2024

pablomartin4btc Nov 4, 2024

pablomartin4btc Nov 6, 2024

vasild Nov 2, 2024

pablomartin4btc Nov 5, 2024

pablomartin4btc Nov 6, 2024

pablomartin4btc Nov 14, 2024

vasild Nov 15, 2024 •

edited

Loading

pablomartin4btc Nov 15, 2024

pablomartin4btc commented Nov 6, 2024

pablomartin4btc commented Nov 10, 2024

vasild left a comment

pablomartin4btc commented Nov 11, 2024

vasild left a comment

pablomartin4btc commented Nov 12, 2024

D33r-Gee left a comment

pablomartin4btc commented Nov 14, 2024

vasild left a comment

MarnixCroes left a comment

pablomartin4btc commented Nov 25, 2024 •

edited

Loading

GBKS commented Dec 3, 2024

pablomartin4btc commented Dec 3, 2024

johnny9 Dec 4, 2024

pablomartin4btc Dec 5, 2024

johnny9 Dec 4, 2024

pablomartin4btc Dec 5, 2024

johnny9 Dec 4, 2024 •

edited

Loading

pablomartin4btc Dec 5, 2024

GBKS commented Dec 4, 2024

D33r-Gee commented Dec 4, 2024


		CService serv(LookupNumeric(ipAddress, DEFAULT_PROXY_PORT));

		Proxy addrProxy = Proxy(serv, true);

Allow IPv6 in Proxy settings and moving validation out from the UI into the model/ interface #430

Are you sure you want to change the base?

Allow IPv6 in Proxy settings and moving validation out from the UI into the model/ interface #430

Conversation

pablomartin4btc commented Nov 1, 2024 • edited Loading

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablomartin4btc commented Nov 6, 2024

pablomartin4btc commented Nov 10, 2024

vasild left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Nov 11, 2024

vasild left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Nov 12, 2024

D33r-Gee left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Nov 14, 2024

vasild left a comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Nov 25, 2024 • edited Loading

GBKS commented Dec 3, 2024

pablomartin4btc commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnny9 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GBKS commented Dec 4, 2024

D33r-Gee commented Dec 4, 2024

pablomartin4btc commented Nov 1, 2024 •

edited

Loading

vasild Nov 15, 2024 •

edited

Loading

pablomartin4btc commented Nov 25, 2024 •

edited

Loading

johnny9 Dec 4, 2024 •

edited

Loading